Add ability to generate buildkite pipelines from integration testing framework #5391
Conversation
blakerouse
added
Team:Elastic-Agent-Control-Plane
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
|
|
|
mergify
bot
added
the
backport-8.x
pazone
force-pushed
the
integration-buildkite
branch
from
50878e6 to
9ccdb9b
Compare
There was a problem hiding this comment.
I'm a little worried seeing more code making its way into magefile.go and the pkg/testing package.
The reason that the code has been added there is that there is already part of the functionality in magefile and this is pulling in more code, code that we cannot easily test/refactor.
There's nothing technically wrong with the PR, but having the source code tightly coupled with elastic-agent CI tool in pkg/testing looks like a dependency in the wrong direction.
magefile.go
Outdated
| return fmt.Errorf("failed to create runner: %w", err) | ||
| } | ||
|
|
||
| steps, err := r.Buildkite() |
There was a problem hiding this comment.
Why is Buildkite() a method of a runner ?
I understand that there's some convenient code already there but is generating buildkite steps a responsibility of a runner ?
Should an integration test runner know about Buildkite and its step definition?
This looks like a code smell as the runner is probably doing too much
Perhaps the runner logic should be broken up in smaller independent pieces ?
Edit:
What about extracting the batching functionality and dumping the batches in a structured manner so that the buildkite steps can be built from that ?
That should be something that enables the dynamic creation of buildkite steps without introducing tight coupling between integration test runners and buildkite
There was a problem hiding this comment.
I prefer it this way otherwise I would have to expose more internals of the runner for the buildkite runner. Generating buildkite is a unique way of running the testing runner, but either way its still the runner performing the work its just being done through buildkite.
My overall goal of this PR was just to show that it can be done and not something that needed to be hard coded in YAML. The actually work of making this work will be done by @pazone
There was a problem hiding this comment.
@pazone Okay, I just went ahead and refactored the runner to allow me to pull it out and not create the runner as you requested. Should be good now.
|
This pull request is now in conflicts. Could you fix it? 🙏 |
blakerouse
force-pushed
the
integration-buildkite
branch
from
74235c1 to
73f921a
Compare
|
buildkite test this |
There was a problem hiding this comment.
Thanks @blakerouse , I appreciate the effort of breaking up runner into smaller pieces even if it is mostly moving structs and interfaces to other packages.
It looks better than the previous iteration, I am still a bit dismayed that, while some functions have relocated to magefile.go, structs and tests specific to buildkite live under pkg/testing/buildkite.
What I meant in my previous review when talking about breaking up some of the runner functionality was:
- expose batching as a free function of some kind
- have it return a structured output (not buildkite-specific, just generic grouping of tests on different platforms) for the batches that need to run
- envelope the operation in a mage target and have buildkite figure out what steps to run in the pipeline based on that output.
Would that be feasible in this or in a follow-up PR?
Done in this PR as requested: https://github.com/elastic/elastic-agent/pull/5391/files#diff-a19232eca67fe3c99e1ee41e39cece7dcf26dc8ba648cc6784b4bbdd14aa2e16R15
Done in this PR: https://github.com/elastic/elastic-agent/pull/5391/files#diff-a19232eca67fe3c99e1ee41e39cece7dcf26dc8ba648cc6784b4bbdd14aa2e16R34 This is not buildkite specific in anyway.
Done here in its own module (adding less to the magefile which is already too large): https://github.com/elastic/elastic-agent/pull/5391/files#diff-76ca66738b839b0b344d18df21e58cdc31fd9a9cd9c77365e3e57193cf1483a7R204
I did everything requested in your previous comment and I have everything already in this PR you are requesting again. |
|
buildkite test this |
1 similar comment
|
buildkite test this |
|
QQ: can we use build tags like |
|
I am concerned by the complexity of the YAML generation process. It's good that it generates the steps but it's hard to troubleshoot and modify that. |
|
…framework (#5391) Adds the ability to generate the integration testing workload as buildkite pipelines. This doesn't actually complete the enablement. This just provides a great starting point for generating the buildkite steps. More work is still to be done to actually perform the work in the steps. (cherry picked from commit c645a5a)
…framework (#5391) (#5630) Adds the ability to generate the integration testing workload as buildkite pipelines. This doesn't actually complete the enablement. This just provides a great starting point for generating the buildkite steps. More work is still to be done to actually perform the work in the steps. (cherry picked from commit c645a5a) Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
What does this PR do?
Adds the ability to generate the integration testing workload as buildkite pipelines.
This doesn't actually complete the enablement. This just provides a great starting point for generating the buildkite steps. More work is still to be done to actually perform the work in the steps.
Why is it important?
Ensures that all testing logic is still defined in golang test files, but allows the work to be performed by buildkite runners. This removes the need for the integration tests to spin up GCP instances, but doesn't stop that from happening when developers need to run the tests locally.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works[ ] I have added an entry in./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E test